-
-
Notifications
You must be signed in to change notification settings - Fork 33.5k
bpo-40389: Improve repr of typing.Optional #19714
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
Lib/typing.py
Outdated
|
|
||
| def __repr__(self): | ||
| if (self.__origin__ == Union and len(self.__args__) == 2 | ||
| and self.__args__[1] is type(None)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Endilll, thanks for opening a pull request for this. Notice that this does not work, when None is the first element:
>>> import typing
>>> typing.Union[None, int]
typing.Union[NoneType, int]
>>> typing.Union[None, int] == typing.Optional[int]
True
This change will also need some tests and an NEWS entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback. Fixed.
|
You can add a NEWS entry using https://blurb-it.herokuapp.com/ |
|
Thanks for the PR and welcome @Endilll. FYI: assuming the idea is approved by the maintainers of |
ilevkivskyi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks! Also be sure to sign the CLA.
The Azure failure look like a flake, but I can't re-trigger it, and it is required for merging. @zooba do you know how to fix this?
gvanrossum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please sign the CLA so we can merge.
|
I've submitted CLA back on Saturday, and bot says that one business day is needed to update the records, so I suppose we just need to wait a bit. |
|
Closing and reopening to re-run the Azure pipelines. |
|
@gvanrossum: Please replace |
|
Congratulations! |
|
I want to thank everybody for such a welcoming attitude. |
https://bugs.python.org/issue40389